Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce stack usage #190

Merged
merged 24 commits into from
Dec 22, 2023
Merged

Reduce stack usage #190

merged 24 commits into from
Dec 22, 2023

Conversation

rmja
Copy link
Contributor

@rmja rmja commented Dec 7, 2023

This is a proposal to write request to a re-useable buffer instead of allocating the heapless Vec on the stack.
If this is something that we decide to go with, then we should also do something to reuse the same buffer when parsing the response.
Note, there is a todo in serde_at/src/ser/mod.rs when compiled without heapless.

@rmja
Copy link
Contributor Author

rmja commented Dec 7, 2023

@MathiasKoch please have a look :)

@rmja rmja marked this pull request as ready for review December 8, 2023 13:11
@MathiasKoch
Copy link
Member

MathiasKoch commented Dec 11, 2023

I am not sure if I understand how this reduces stack usage? It makes it easier to be certain you have enough stack available, as you do the allocation up front, but wouldn't you still need the same size, with the limitation that it is much harder to calculate now, as you need to know up from what the serialized size of your biggest command + arguments is throughout the entire program?

@MathiasKoch
Copy link
Member

@rmja Is this still relevant?

@rmja
Copy link
Contributor Author

rmja commented Dec 19, 2023

Yes, I believe so. This PR also removed 2k of my code size. Please come with any good ideas on how to improve it further.

@rmja
Copy link
Contributor Author

rmja commented Dec 19, 2023

@dragonnn could you verify that this in fact reduce the stack usage? I known that we still need to handle the stack usage of the response.

@dragonnn
Copy link
Contributor

Yes, it does. Helps already a lot, thanks :)

@rmja
Copy link
Contributor Author

rmja commented Dec 20, 2023

@dragonnn awesome! The last few commits depends on embassy-rs/embassy#2328.

@MathiasKoch
Copy link
Member

@rmja This looks nice!

Could you fix the CI?

@rmja
Copy link
Contributor Author

rmja commented Dec 21, 2023

Ok, I need some help.
I have updated CI to now also test with async feature. For that to compile, I had to remove these two tests: d8ed5aa

Can anyone help with getting these two tests back?

@MathiasKoch
Copy link
Member

MathiasKoch commented Dec 21, 2023

What is the issue with them? (don't have the option of checking out the branch right now)

@rmja
Copy link
Contributor Author

rmja commented Dec 21, 2023

OMG: CI is green! I would really appreciate if the two of you could have a look again!

@dragonnn
Copy link
Contributor

Works fine in my project, didn't notice any issue and no changes where need from previous commit I tested.

Copy link
Member

@MathiasKoch MathiasKoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Thank you very much for your efforts on this!
Can't wait to try it out on my own devices. Sadly, I still have a bit of way on migrating blocking code to async before I can fully upgrade.

@MathiasKoch MathiasKoch merged commit fe971a4 into FactbirdHQ:master Dec 22, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants